-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding a navigational structure based on aip.dev #1253
base: master
Are you sure you want to change the base?
Adding a navigational structure based on aip.dev #1253
Conversation
Checking out your branch... |
From https://github.com/GoogleCloudPlatform/cloud-opensource-java/wiki/Tools#jekyll-for-github-pages:
It worked. I didn't find any missing link. |
docs/_includes/jlbp-header.html
Outdated
<nav class="glue-header__link-bar"> | ||
<ul id="list-1" class="glue-header__list"> | ||
<li class="glue-header__item{% if page.jlbp or page.jlbp_index %} glue-header--is-active{% endif %}"> | ||
<a class="glue-header__link" href="/" aria-level="1">Browse best practices</a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find this "Browse best practices" link redundant, because the logo "JLBPs" goes to the same top page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is technically redundant, but the explicit link helps people find what they are looking for easier - clicking on the logo might not be obvious. Also, I would argue that we shouldn't couple together the root of the site with the JLBP index - we might want to separate those ideas apart later. Lastly, if we add more on the header later, it will feel more "even" (look at the header of aip.dev).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that button may be useful in future, but not now. We can easily add it back to the site when needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elharo what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking of an idea for headers to add. What about:
- Best practices (the existing one)
- Articles
- Reference
- Tools (new page - discuss the static linkage checker)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. Let's put that in a separate PR though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing a separate PR seems fine for the header links I suggested here - but how about the single header link that is already present ("Browse best practices") which @suztomo wants to remove?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header bar and site search is the part of this PR I'm not sold on. The navigation and sidebar is a much more obvious win. I think we can move that forward quickly without having to vet all the JavaScript that's being added for Tipue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can remove the search functionality if that's a sticking point. However, there is one key problem with removing the whole header - it's what provides the link to navigate to the root of the site, and on mobile, it's the only way to navigate away from the current page (through the hamburger menu).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The results look good. Internally this is a major increase in CSS complexity. I'm not sure I understand it all and I'm going to need some time to look through it, so I'm going to add a temporary hold.
Why do you need to understand it? It's CSS... which I never understand... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unsure about the top bar. Can we move that to a separate PR and make this PR just the sidebar and navigation?
The top bar has the new search functionality, which seems like a very useful feature. At a high level, I tried to minimize the work to create this navigational structure by starting with aip.dev's structure and stripping out stuff we don't need. That process was pretty straightforward. Gutting the top bar would require more work, then we'd need even more work to put back a top bar later when we want one. Basically, I view the current structure in the PR as an MVP, and further changes are improvements on top of it. |
my comnets are:
I actually do like the site as-is, and agreed that it could use a backlink. Proposed left-nav:
|
|
The standard on c.g.c (namely concepts) doesn't mean it's a best practice...
+1 . The index may just be the home page itself as is today.
I do feel articles is misleading; otherwise, articles become a long list of dumping ground for any new content; grouping them logically structure is better, and we can be more focused on what content to add.
I can understand not having Introudction over-shadow the rules themselves. I also dislike a lot of c.g.c article put "intro" or "concepts" at the end ;) How do we group these articles together? I see them as introduction, or just info on Dependency Conflicts.
|
I'm not sure why best practices matter exactly. Consistency itself is a best practice anyway, right? What best practices are you trying to enforce here? I'm not sure why you are referencing "concepts" in particular, besides the fact that it's the link I gave; you can visit any page under cloud.google.com and see the TOC on the right, e.g. https://cloud.google.com/bigquery/docs/reference/standard-sql/data-types . |
What do you think about me using "Concepts" instead of "Articles"? (I'm looking for a short name so that I can stick it on the header later) Do you feel that the article titles should be in the sidebar, or is a link to a Concepts index page sufficient? |
I have made some updates:
PTAL |
@@ -0,0 +1,9 @@ | |||
<!-- Closing scripts --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this script do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lukesneeringer could you help us understand what the scripts in https://github.com/googleapis/aip/blob/master/_includes/aip-closing-scripts.html do and why we might need them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Offline answer:
- The inline one makes the header bar work on mobile.
- The other two are what Glue tells you to use. https://glue-docs.appspot.com/get-started/cdn/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add comments to the file explaining this.
docs/_layouts/default.html
Outdated
</script> | ||
{% endif %} | ||
</head> | ||
<html lang="{{ site.lang | default: 'en-US' }}" class="glue-flexbox"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stet. This is en-US, not site.lang
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you're asking for...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will revert this line.
docs/_sass/callouts.scss
Outdated
border: 1px solid $glue-blue-200; | ||
} | ||
|
||
.tldr { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which of these do we actually use? I don't see .tldr referenced anywhere for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't use them yet, but I thought they could be useful to incorporate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove the callout stuff since it's not used.
docs/_sass/colors.scss
Outdated
@@ -0,0 +1,139 @@ | |||
// This is forked. Do not edit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not? It it's not editable it shouldn't be in the repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what your objection is... it's pretty standard practice to put warnings on files that are derived from other sources. Technically such files can be edited, but it's not recommended because it's much harder to maintain.
I pulled out the page metadata and dynamic index into #1270 . |
Will do. |
When I run this for myself, I see the correct one. This seems to be coming from the GitHub repo and not from anything under the |
Addressed feedback (except one item where I have a question), PTAL |
docs/_layouts/default.html
Outdated
<main role="main" id="page-content"> | ||
{% include jlbp-nav.html %} | ||
<section id="jump-content"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to run the site again, but it looks like there's more navigation inside main, e..g jump-content? Maybe that's the rightand sidebar? Main is for the content only. This is important for accessibility and rendering on non-desktop screens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
PTAL, all navigation stuff has been moved outside of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - Footer updated to black. |
@elharo what browser are you using to check accessibility? |
That was firefox. Couldn't get Chrome to work.
…On Fri, Mar 6, 2020 at 11:09 AM Garrett Jones ***@***.***> wrote:
@elharo <https://github.com/elharo> what browser are you using to check
accessibility?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1253?email_source=notifications&email_token=AAHVP2HCH47EPN7XARR3IILRGEN27A5CNFSM4K4MQ3YKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOB4RFQ#issuecomment-595839126>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAHVP2F2FSPXAUDP5LTUAVTRGEN27ANCNFSM4K4MQ3YA>
.
--
Elliotte Rusty Harold
[email protected]
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Titles in left sidebar are still gray on gray. E.g "Best Practices"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are two title elements in the head.
<html lang="en-US" class="glue-flexbox">
--
| <head>
| <title>Google Best Practices for Java Libraries</title>
| <meta charset='UTF-8'>
| <meta http-equiv="X-UA-Compatible" content="IE=edge">
| <meta name="viewport" content="width=device-width,maximum-scale=2">
| <link href="https://fonts.googleapis.com/css?family=Roboto:100,300,400,500,700\|Google+Sans:400,500\|Product+Sans:400&lang=en" rel="stylesheet"></link>
| <link rel="stylesheet" type="text/css" media="screen" href="https://www.gstatic.com/glue/v22_1/glue.min.css">
| <link rel="stylesheet" type="text/css" media="screen" href="/assets/css/style.css?v=20deac654f99e81a3e313819fe82ba3d87edbb4d">
| <link rel="stylesheet" type="text/css" media="print" href="/assets/css/print.css?v=20deac654f99e81a3e313819fe82ba3d87edbb4d">
| <script type="text/javascript" src="https://code.jquery.com/jquery-3.4.0.min.js"></script>
| <script type="text/javascript" src="https://www.gstatic.com/glue/latest/glue-detect.min.js"></script>
| <script type="text/javascript">
| $('html').css('visibility', 'hidden');
| $.when($.ready).then(() => {
| $('html').css('visibility', 'visible');
| });
| </script>
| <script type="text/javascript" src="/assets/js/global.js?v=20deac654f99e81a3e313819fe82ba3d87edbb4d"></script>
| <!-- Begin Jekyll SEO tag v2.5.0 -->
| <title>Google Best Practices for Java Libraries \| Code health dashboard for GCP open source projects</title>
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed the contrast for highlighted sidebar links. I'm not sure about the keyboard warnings either. I believe they come from things put there by Glue. It all follows the template provided by Glue at https://glue-docs.appspot.com/docs/components/raw/header#HTML . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have both a big View on Github Button and a View Page on Github link?
The link alone is probably sufficient. The primary use case here are readers, not writers and I'd prefer not to put more chart junk on the page.
And once that's done, do we really need the top bar at all? Maybe just move the logo into the top of the lefthand nav bar, and then more of the JLBPs fit above the fold.
docs/_includes/jlbp-footer.html
Outdated
<!-- prettier-ignore --> | ||
<footer> | ||
{% if site.google_analytics %} | ||
<p>This site uses cookies from Google to deliver its services and to analyze traffic. (<a href='https://policies.google.com/technologies/cookies'>learn more</a>)</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
learn --> Learn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
I removed the "View on Github" button, and I found some more dead css to remove. We still need the top bar:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firefox reports four remaining accessibility issues.
<!-- LOCK up--> | ||
<div class="glue-header__container"> | ||
<div class="glue-header__lock-up"> | ||
<div class="glue-header__hamburger glue-header__hamburger--first-tier"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem to show up and it issues an accessibility warning:
"Clickable elements must be focusable and should have interactive semantics. Learn more"
Can we delete it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken straight from the Glue sample:
https://glue-docs.appspot.com/docs/components/raw/header#HTML
Also it only appears on mobile mode. Shrink the window down to see it appear.
<div class="glue-header__lock-up"> | ||
<div class="glue-header__hamburger glue-header__hamburger--first-tier"> | ||
<div class="glue-header__hamburger-wrapper"> | ||
<button type="button" class="glue-header__drawer-toggle-btn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Warning
Focusable elements should have interactive semantics."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken straight from the Glue sample:
https://glue-docs.appspot.com/docs/components/raw/header#HTML
<p class="glue-header__logo--product">JLBPs</p> | ||
</a> | ||
</div> | ||
<a href="#page-content" class="glue-header__skip-content"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Error
Interactive elements must be labeled. Learn more"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken straight from the Glue sample: https://glue-docs.appspot.com/docs/components/raw/header#HTML
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I don't trust Glue as much as you do. When Firefox scans aip.dev it finds a number of problems I know are real issues, so I'm inclined to give it the benefit of the doubt on the ones I don't immediately understand. If you like, we can dig into these further, though in general I'm more comfortable building up from small pieces of code I understand than trying to cut out the large pieces I don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are problems with Glue, I think we should get them fixed at the source instead of 1) trying hack fixes on top of Glue or 2) forging our own path. I would argue that fixing problems at the source should not be in the scope of this PR and shouldn't block it, given the value-add nature of the PR. Are you going to set a bar of perfection in order to approve the PR? We are not a front-end team; we are a team trying to help people solve diamond dependency conflicts.
I would guess that the accessibility issues on aip.dev that have been fixed on jlbp.dev are likely local choices and not issues with Glue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of these issues are from the sample html that Glue provides. Glue is designed as an accessible framework. I don't want to second-guess Glue given my lack of experience in this area. I'm guessing Firefox isn't perfect here. For example, two of the accessibility items are about the hamburger menu, which is supposed to be absent in the desktop view.
<p class="glue-header__logo--product">JLBPs</p> | ||
</a> | ||
</div> | ||
<a href="#page-content" class="glue-header__skip-content"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken straight from the Glue sample: https://glue-docs.appspot.com/docs/components/raw/header#HTML
<!-- LOCK up--> | ||
<div class="glue-header__container"> | ||
<div class="glue-header__lock-up"> | ||
<div class="glue-header__hamburger glue-header__hamburger--first-tier"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken straight from the Glue sample:
https://glue-docs.appspot.com/docs/components/raw/header#HTML
Also it only appears on mobile mode. Shrink the window down to see it appear.
<div class="glue-header__lock-up"> | ||
<div class="glue-header__hamburger glue-header__hamburger--first-tier"> | ||
<div class="glue-header__hamburger-wrapper"> | ||
<button type="button" class="glue-header__drawer-toggle-btn" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken straight from the Glue sample:
https://glue-docs.appspot.com/docs/components/raw/header#HTML
fixes #885
fixes #992
fixes #1034